-
Notifications
You must be signed in to change notification settings - Fork 216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove some access.hpp includes in overlay #1205
Remove some access.hpp includes in overlay #1205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -741,7 +740,7 @@ struct get_turns_cs | |||
else if (c < left) return -1; | |||
else if (c > right) return 1; | |||
else return 0; | |||
} | |||
}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this one. I don't remember exactly but I probably should have cleaned it up earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the get_side function that was commented out and the commented out that was calling it. If the latter was meant to be kept, I can change the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is completely fine, thanks!
@@ -16,7 +16,7 @@ | |||
|
|||
|
|||
#include <boost/geometry/algorithms/detail/intersection/interface.hpp> | |||
#include <boost/geometry/algorithms/detail/overlay/intersection_box_box.hpp> | |||
#include <boost/geometry/algorithms/detail/intersection/box_box_implementation.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this? I'm not against, but we don't have this convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for moving the code of intersection_box_box.hpp out of overlay was that it was not used there directly. detail/intersection-directory seemed like a better fit.
The reason for not moving the code into in ./algorithms/detail/intersection/box_box.hpp was that I could not include that header into ./index/detail/algorithms/intersection_content.hpp, where the code is also used, without test failures due other dependencies between relate and rtree. I did not try to resolve those because it seemed sensible that the content of intersection_box_box.hpp, which looked useful and in itself not dependent on anything non-core, would remain in its own header file. It also did not look like its logic could ever be CS-dependent or warrant an alternative implementation, so I did not move it into a strategy.
The reason for appending _implementation was that the name box_box.hpp was already taken in that directory by the header with the dispatch code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, clear, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks LGTM.
This removes the include of access.hpp from some files in the overlay algorithm that don't use it (AFAICS they don't use get, set or the {max,min}_corner constants) and moves the box_box_intersection implementation out of the overlay algorithm where it is not used directly.
(This is a by-product of my larger attempt to locate and separate direct computations involving coordinates from the overlay-code, which I think is a prerequisite for implementing and testing numerically robust strategies. Because I don't know how long that might take, whether it will succeed and how much the develop-branch might change in the mean time, I thought it made sense to push this self-contained change early.)